-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add setRequestDecoder to ResponseEncoder interface #24368
Conversation
Signed-off-by: Paul Sohn <paulsohn@google.com>
/assign @danzh2010 |
/assign @alyssawilk |
/retest |
Retrying Azure Pipelines: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo one of two explanatory comments :-)
|
||
/** | ||
* Set a new request decoder for this ResponseEncoder. | ||
* @param decoder new request decoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think it's worth a comment on why this is a useful association?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, ptal!
@@ -156,6 +156,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { | |||
return stream_error_on_invalid_http_message_; | |||
} | |||
|
|||
void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe why HTTP/1.1 doesn't need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. H/1 will need more work to actually reset the decoder on internal redirect, but it seems like we don't need to tackle that now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to make it work for H1 to defer logging I guess? Can you add a TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this part out! One more request, could you move the recreateStream()
change to this PR as well?
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Done. |
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
CC @envoyproxy/mobile-maintainers: FYI only for changes made to |
Signed-off-by: Paul Sohn <paulsohn@google.com>
// This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active | ||
// stream's pointer to the RequestDecoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own knowledge, how does H1 codec decode request headers in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
H1 has an ActiveStream that has a handle on both a ResponseEncoder and RequestDecoder. So e.g. on receiving headers the connection goes through the active request to get to the decoder, like here. It seems like the H/2 and H/3 codecs have the decoding path go through the response encoder -> request decoder partly to simplify multiplexing, so it makes sense that H/1 (which doesn't have streams) is organized a bit differently. I could be wrong about that last part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get it! Thanks for the link!
@@ -156,6 +156,8 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder { | |||
return stream_error_on_invalid_http_message_; | |||
} | |||
|
|||
void setRequestDecoder(Http::RequestDecoder& /*decoder*/) override {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to make it work for H1 to defer logging I guess? Can you add a TODO?
Signed-off-by: Paul Sohn <paulsohn@google.com>
cc @mattklein123 for @google pass (circleci failure can be ignored) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please update the PR description to reflect the recreateStream() change.
// This doesn't currently work for HTTP/1 as the H/1 ResponseEncoder doesn't hold the active | ||
// stream's pointer to the RequestDecoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get it! Thanks for the link!
….h-into-multiple-header-files * origin/main: Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) Signed-off-by: JP Simard <jp@jpsim.com>
…-cpp-to-latest-version * origin/main: (23 commits) Reduce Route memory utilization by avoiding RuntimeData instances when not needed (#24327) build: fix compile error for mac (#24429) postgres: support for upstream SSL (#23990) iOS: split `EnvoyEngine.h` into multiple header files (#24397) mobile: check for pending exceptions after JNI call (#24361) Remove uneccessary `this->` from mobile engine builder (#24389) Add setRequestDecoder to ResponseEncoder interface (#24368) downstream: refactoring code to remove listener hard deps (#24394) lb api: moving load balancing policy specific configuration to extension configuration (#23967) ci: Skip docker/examples verification for docs or mobile only changes (#24417) ci: run mobile GitHub Actions on every PR (#24407) mobile: remove `bump_lyft_support_rotation.sh` script (#24404) Add file size to DirectoryEntry (#24176) bazel: update to 6.0.0rc4 (#24235) bazel: update rules_rust (#24409) Ecds config dump recommit (#24384) bazel: add another config_setting incompatible flag (#24270) listeners: moving listeners to extension directory (#24248) mobile: build Swift with whole module optimization (#24396) ci: update `actions/setup-java` from v1 to v3.8 (#24393) ... Signed-off-by: JP Simard <jp@jpsim.com>
Commit Message: Add setRequestDecoder to ResponseEncoder interface
Additional Description: We also set the new request decoder in recreateStream. This change was introduced in #23648 but was moved into a separate PR given its more general scope.
Risk Level: Low
Testing: No tests added.
Docs Changes: N/A
Release Notes: N/A